- 
                Notifications
    You must be signed in to change notification settings 
- Fork 421
          Put a 10_000vByte cap on HolderHTLCOutput 0FC package templates
          #4129
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
  
    Put a 10_000vByte cap on HolderHTLCOutput 0FC package templates
  
  #4129
              Conversation
| 👋 Thanks for assigning @wpaulino as a reviewer! | 
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #4129      +/-   ##
==========================================
+ Coverage   88.63%   88.84%   +0.21%     
==========================================
  Files         180      180              
  Lines      134878   137870    +2992     
  Branches   134878   137870    +2992     
==========================================
+ Hits       119543   122490    +2947     
- Misses      12567    12568       +1     
- Partials     2768     2812      +44     
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
        
          
                lightning/src/chain/package.rs
              
                Outdated
          
        
      | // See rust-bitcoin to_vbytes_ceil | ||
| let self_vbytes = (self.weight() + 3) / 4; // This is the weight of the witnesses alone, we need to add more here | ||
| let other_vbytes = (other.weight() + 3) / 4; | ||
| // What is a good offset to use here to leave room for the user-provided input-output pair? | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmmmmmmm, I wonder if we shouldn't just not merge and then do the work in the events::bump_transaction module and add like a needs_v3_transaction: Option<()> in the event? That way its at least pretty explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean aggregate in bump_transaction ? How about restrict ourselves to aggregate max 25 HTLCs ? This should be good for any conceivable user-provided input-output pair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No as in break an aggregation in bump_transaction
63e8f1c    to
    db273bb      
    Compare
  
    db273bb    to
    107eb25      
    Compare
  
    | 🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. | 
| // Cap the size of transactions claiming `HolderHTLCOutput` in 0FC channels. | ||
| // Otherwise, we could hit the max 10_000vB size limit on V3 transactions | ||
| // (BIP 431 rule 4). | ||
| 25 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is okay, but I thought the point of pulling this out into the bump_transaction logic is so that we could do the split after we see the inputs the user wants to contribute so that we can get close to 10k without exceeding it, rather than picking some random "max overhead" constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks yea let me know the commit below sounds
107eb25    to
    c222c41      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approach lgtm
| let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight; | ||
| if expected_signed_tx_weight >= max_weight { | ||
| let extra_weight = expected_signed_tx_weight - max_weight; | ||
| let htlcs_to_remove = (extra_weight / chan_utils::htlc_timeout_tx_weight(channel_type) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical, but I think these weights include transaction overhead (version etc) so we'll overcount by 40 wu per htlc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we should just use the weight of the HTLC input-output pair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks ! yes this likely resolves the weird + 2 I've got below
| if expected_signed_tx_weight >= max_weight { | ||
| let extra_weight = expected_signed_tx_weight - max_weight; | ||
| let htlcs_to_remove = (extra_weight / chan_utils::htlc_timeout_tx_weight(channel_type) | ||
| // If we remove extra_weight / timeout_weight + 1 we sometimes still land above max_weight | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this wouldn't be an issue if we just round up the integer division?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely it's because htlc_timeout_tx_weight is the weight of the full-transaction, but we want to only count the bytes from the single input-output pair
| // (BIP 431 rule 4). | ||
| chan_utils::TRUC_MAX_WEIGHT | ||
| } else { | ||
| u64::MAX | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well cap this to the max weight we can relay?
| engine.input(&htlc.commitment_txid.to_byte_array()); | ||
| engine.input(&htlc.htlc.transaction_output_index.unwrap().to_be_bytes()); | ||
| } | ||
| let utxo_id = ClaimId(Sha256::from_engine(engine).to_byte_array()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want to reuse the same ID until we move on to the next batch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, was just wondering this - should we use the one from the event at least until we split the batch, which would work in the common case. Might want to sort the outputs so that we use them in a deterministic order (I imagine they are in the event, but...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed below, don't quiet yet sort the HTLCs, need to look into whether they are already deterministic in the event.
8d2b31c    to
    4d54289      
    Compare
  
    | @wpaulino Carla raised this question: what do we do for the 1000vB limit on the anchor transaction ? For now I opted for documentation + debug assertion in 952a6b1 | 
3ed04aa    to
    92d1ce7      
    Compare
  
    | } | ||
| while !selected_utxos.is_empty() | ||
| && selected_amount - selected_utxos.front().unwrap().0.output.value | ||
| >= target_amount_sat + total_fees | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be total_fees - selected_utxos.front().unwrap().1 to also remove the fees for this utxo if we're going to drop it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes ! thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, two non-blocking micro-nits
| .ok_or_else(|| { | ||
| log_debug!( | ||
| self.logger, | ||
| "max_tx_weight is too small to accomodate the preexisting tx weight plus a P2WSH/P2TR output" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/accomodate/accommodate
| /// 4. The final transaction must have a weight smaller than `max_tx_weight`; if this | ||
| /// constraint can't be met, return an `Err`. In the case of counterparty-signed HTLC | ||
| /// transactions, we will remove a chunk of HTLCs and try your algorithm again. Anchor | ||
| /// transactions cannot be trimmed, so if you fail to meet the `max_tx_weight` we will | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "fail to meet max_tx_weight" makes it sound like this is a goal that we want to reach, rather than a limit we should stay under.
bdfedc4    to
    386075d      
    Compare
  
    | 🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM. We also need to update the documentation on BumpTransactionEvent.
| for (utxo, fee_to_spend_utxo) in eligible_utxos { | ||
| if selected_amount >= target_amount_sat + total_fees { | ||
| if selected_amount >= target_amount_sat + total_fees | ||
| && selected_utxos_weight <= max_coin_selection_weight | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second condition should always be true, I believe (and is in tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right I added this condition to guard against a single UTXO exhausting the max_coin_selection_weight - unlikely but possible i'm thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISTM that'd be more readable as
if utxo.satisfaction_weight >= max_coin_selection_weight { continue; } :)
        
          
                lightning/src/chain/mod.rs
              
                Outdated
          
        
      | } | ||
| ClaimId(Sha256::from_engine(engine).to_byte_array()) | ||
| } | ||
| pub(crate) fn with_bytes(&self, bytes: &[u8]) -> ClaimId { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be useful to clarify what this is doing more in the name. Like modify_with_bytes or step_with_bytes?
| Go ahead and address all the pending comments and re-squash, IMO. | 
386075d    to
    d8e0319      
    Compare
  
    | 
 diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs
index 89b523ece..b4cc6a302 100644
--- a/lightning/src/chain/mod.rs
+++ b/lightning/src/chain/mod.rs
@@ -455,7 +455,7 @@ impl ClaimId {
 		}
 		ClaimId(Sha256::from_engine(engine).to_byte_array())
 	}
-	pub(crate) fn with_bytes(&self, bytes: &[u8]) -> ClaimId {
+	pub(crate) fn step_with_bytes(&self, bytes: &[u8]) -> ClaimId {
 		let mut engine = Sha256::engine();
 		engine.input(&self.0);
 		engine.input(bytes);
diff --git a/lightning/src/events/bump_transaction/mod.rs b/lightning/src/events/bump_transaction/mod.rs
index ec214556d..651338383 100644
--- a/lightning/src/events/bump_transaction/mod.rs
+++ b/lightning/src/events/bump_transaction/mod.rs
@@ -127,12 +127,21 @@ pub enum BumpTransactionEvent {
 	/// feerate of the commitment transaction is already sufficient, in which case the child anchor
 	/// transaction is not needed and only the commitment transaction should be broadcast.
 	///
+	/// In zero-fee commitment channels, the commitment transaction and the anchor transaction
+	/// form a 1-parent-1-child package that conforms to BIP 431 (known as TRUC transactions).
+	/// The anchor transaction must be version 3, and its size must be no more than 1000 vB.
+	/// The anchor transaction is usually needed to bump the fee of the commitment transaction
+	/// as the commitment transaction is not explicitly assigned any fees. In those cases the
+	/// anchor transaction must be broadcast together with the commitment transaction as a
+	/// `child-with-parents` package (usually using the Bitcoin Core `submitpackage` RPC).
+	///
 	/// The consumer should be able to sign for any of the additional inputs included within the
-	/// child anchor transaction. To sign its anchor input, an [`EcdsaChannelSigner`] should be
-	/// re-derived through [`SignerProvider::derive_channel_signer`]. The anchor input signature
+	/// child anchor transaction. To sign its keyed-anchor input, an [`EcdsaChannelSigner`] should
+	/// be re-derived through [`SignerProvider::derive_channel_signer`]. The anchor input signature
 	/// can be computed with [`EcdsaChannelSigner::sign_holder_keyed_anchor_input`], which can then
 	/// be provided to [`build_keyed_anchor_input_witness`] along with the `funding_pubkey` to
-	/// obtain the full witness required to spend.
+	/// obtain the full witness required to spend. Note that no signature or witness data is
+	/// required to spend the keyless anchor used in zero-fee commitment channels.
 	///
 	/// It is possible to receive more than one instance of this event if a valid child anchor
 	/// transaction is never broadcast or is but not with a sufficient fee to be mined. Care should
@@ -183,14 +192,25 @@ pub enum BumpTransactionEvent {
 		pending_htlcs: Vec<HTLCOutputInCommitment>,
 	},
 	/// Indicates that a channel featuring anchor outputs has unilaterally closed on-chain by a
-	/// holder commitment transaction and its HTLC(s) need to be resolved on-chain. With the
-	/// zero-HTLC-transaction-fee variant of anchor outputs, the pre-signed HTLC
-	/// transactions have a zero fee, thus requiring additional inputs and/or outputs to be attached
-	/// for a timely confirmation within the chain. These additional inputs and/or outputs must be
-	/// appended to the resulting HTLC transaction to meet the target feerate. Failure to meet the
-	/// target feerate decreases the confirmation odds of the transaction, possibly resulting in a
-	/// loss of funds. Once the transaction meets the target feerate, it must be signed for and
-	/// broadcast by the consumer of the event.
+	/// holder commitment transaction and its HTLC(s) need to be resolved on-chain. In all such
+	/// channels, the pre-signed HTLC transactions have a zero fee, thus requiring additional
+	/// inputs and/or outputs to be attached for a timely confirmation within the chain. These
+	/// additional inputs and/or outputs must be appended to the resulting HTLC transaction to
+	/// meet the target feerate. Failure to meet the target feerate decreases the confirmation
+	/// odds of the transaction, possibly resulting in a loss of funds. Once the transaction
+	/// meets the target feerate, it must be signed for and broadcast by the consumer of the
+	/// event.
+	///
+	/// In zero-fee commitment channels, you must set the version of the HTLC claim transaction
+	/// to version 3 as the counterparty's signature commits to the version of
+	/// the transaction. You must also make sure that this claim transaction does not grow
+	/// bigger than 10,000 vB, the maximum vsize of any TRUC transaction as specified in
+	/// BIP 431. It is possible for [`htlc_descriptors`] to be long enough such
+	/// that claiming all the HTLCs therein in a single transaction would exceed this limit.
+	/// In this case, you must claim all the HTLCs in [`htlc_descriptors`] using multiple
+	/// transactions. Finally, note that while HTLCs in zero-fee commitment channels no
+	/// longer have the 1 CSV lock, LDK will still emit this event only after the commitment
+	/// transaction has 1 confirmation.
 	///
 	/// The consumer should be able to sign for any of the non-HTLC inputs added to the resulting
 	/// HTLC transaction. To sign HTLC inputs, an [`EcdsaChannelSigner`] should be re-derived
@@ -211,6 +231,7 @@ pub enum BumpTransactionEvent {
 	///
 	/// [`EcdsaChannelSigner`]: crate::sign::ecdsa::EcdsaChannelSigner
 	/// [`EcdsaChannelSigner::sign_holder_htlc_transaction`]: crate::sign::ecdsa::EcdsaChannelSigner::sign_holder_htlc_transaction
+	/// [`htlc_descriptors`]: `BumpTransactionEvent::HTLCResolution::htlc_descriptors`
 	HTLCResolution {
 		/// The `channel_id` of the channel which has been closed.
 		channel_id: ChannelId,
@@ -500,13 +521,16 @@ where
 				preexisting_tx_weight,
 			));
 			selected_utxos = VecDeque::new();
+			// Invariant: `selected_utxos_weight` is never greater than `max_coin_selection_weight`
 			let mut selected_utxos_weight = 0;
 			for (utxo, fee_to_spend_utxo) in eligible_utxos {
-				if selected_amount >= target_amount_sat + total_fees
-					&& selected_utxos_weight <= max_coin_selection_weight
-				{
+				if selected_amount >= target_amount_sat + total_fees {
 					break;
 				}
+				// First skip any UTXOs with prohibitive satisfaction weights
+				if BASE_INPUT_WEIGHT + utxo.satisfaction_weight > max_coin_selection_weight {
+					continue;
+				}
 				// If adding this UTXO to `selected_utxos` would push us over the
 				// `max_coin_selection_weight`, remove UTXOs from the front to make room
 				// for this new UTXO.
@@ -526,9 +550,7 @@ where
 				selected_utxos_weight += BASE_INPUT_WEIGHT + utxo.satisfaction_weight;
 				selected_utxos.push_back((utxo.clone(), fee_to_spend_utxo));
 			}
-			if selected_amount < target_amount_sat + total_fees
-				|| selected_utxos_weight > max_coin_selection_weight
-			{
+			if selected_amount < target_amount_sat + total_fees {
 				log_debug!(
 					self.logger,
 					"Insufficient funds to meet target feerate {} sat/kW while remaining under {} WU",
@@ -1055,7 +1077,7 @@ where
 			};
 			broadcasted_htlcs += batch_size;
 			batch_size = htlc_descriptors.len() - broadcasted_htlcs;
-			utxo_id = claim_id.with_bytes(&broadcasted_htlcs.to_be_bytes());
+			utxo_id = claim_id.step_with_bytes(&broadcasted_htlcs.to_be_bytes());
 
 			#[cfg(debug_assertions)]
 			let input_satisfaction_weight: u64 =
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index d7a13d59e..cccbabbbd 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -1604,8 +1604,10 @@ pub enum Event {
 	/// Indicates that a transaction originating from LDK needs to have its fee bumped. This event
 	/// requires confirmed external funds to be readily available to spend.
 	///
-	/// LDK does not currently generate this event unless the
-	/// [`ChannelHandshakeConfig::negotiate_anchors_zero_fee_htlc_tx`] config flag is set to true.
+	/// LDK does not currently generate this event unless either the
+	/// [`ChannelHandshakeConfig::negotiate_anchors_zero_fee_htlc_tx`] or the
+	/// [`ChannelHandshakeConfig::negotiate_anchor_zero_fee_commitments`] config flags are set to
+	/// true.
 	/// It is limited to the scope of channels with anchor outputs.
 	///
 	/// # Failure Behavior and Persistence
@@ -1613,6 +1615,7 @@ pub enum Event {
 	/// returning `Err(ReplayEvent ())`), but will only be regenerated as needed after restarts.
 	///
 	/// [`ChannelHandshakeConfig::negotiate_anchors_zero_fee_htlc_tx`]: crate::util::config::ChannelHandshakeConfig::negotiate_anchors_zero_fee_htlc_tx
+	/// [`ChannelHandshakeConfig::negotiate_anchor_zero_fee_commitments`]: crate::util::config::ChannelHandshakeConfig::negotiate_anchor_zero_fee_commitments
 	BumpTransaction(BumpTransactionEvent),
 	/// We received an onion message that is intended to be forwarded to a peer
 	/// that is currently offline. This event will only be generated if the | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some minor comments
| tx | ||
| } | ||
|  | ||
| pub fn provide_anchor_utxo_reserves<'a, 'b, 'c>( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provide_anchor_reserves should call this now
| // Once we've selected enough UTXOs to cover `target_amount_sat + total_fees`, | ||
| // we may be able to remove some small-value ones while still covering | ||
| // `target_amount_sat + total_fees`. | ||
| while !selected_utxos.is_empty() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if this wouldn't always remove them when we don't mind having some additional weight for the benefit of consolidating some small UTXOs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheBlueMatt discussed with Wilmer offline, we thought we could delete this while loop entirely. I have a slight concern about consolidating too aggressively - these small UTXOs could have been enough to bump the next smaller HTLC batch, but if we delete this while loop all these small UTXOs would be spent into the current batch without changing whether the claim bump was successful or not. @wpaulino feel free to correct / expand :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like having it. Unless we have some concept of "is the current fee low or is it average, consolidating UTXOs when we don't need to isn't super nice.
d8e0319    to
    9ddf925      
    Compare
  
    Otherwise, we could hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4). Also introduce a `max_tx_weight` parameter to `select_confirmed_utxos`. This constraint makes sure anchor and HTLC transactions in 0FC channels satisfy the `TRUC_MAX_WEIGHT` and the `TRUC_CHILD_MAX_WEIGHT` maximums. Expand the coin-selection algorithm provided for any `T: WalletSource` to satisfy this new constraint.
9ddf925    to
    4e4a494      
    Compare
  
    | 
 diff --git a/lightning/src/events/bump_transaction/mod.rs b/lightning/src/events/bump_transaction/mod.rs
index 651338383..92fcea5a7 100644
--- a/lightning/src/events/bump_transaction/mod.rs
+++ b/lightning/src/events/bump_transaction/mod.rs
@@ -1063,11 +1063,9 @@ where
 			{
 				Ok(selection) => selection,
 				Err(()) => {
-					let min_weight_to_remove = USER_COINS_WEIGHT_BUDGET;
-					let htlcs_to_remove = min_weight_to_remove
-						/ chan_utils::aggregated_htlc_timeout_input_output_pair_weight(
-							channel_type,
-						) + 1;
+					let htlcs_to_remove = USER_COINS_WEIGHT_BUDGET.div_ceil(
+						chan_utils::aggregated_htlc_timeout_input_output_pair_weight(channel_type),
+					);
 					batch_size = batch_size.checked_sub(htlcs_to_remove as usize).ok_or(())?;
 					if batch_size == 0 {
 						return Err(());
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index b8565fcaa..884e71784 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -403,25 +403,7 @@ fn do_connect_block_without_consistency_checks<'a, 'b, 'c, 'd>(
 }
 
 pub fn provide_anchor_reserves<'a, 'b, 'c>(nodes: &[Node<'a, 'b, 'c>]) -> Transaction {
-	let mut output = Vec::with_capacity(nodes.len());
-	for node in nodes {
-		output.push(TxOut {
-			value: Amount::ONE_BTC,
-			script_pubkey: node.wallet_source.get_change_script().unwrap(),
-		});
-	}
-	let tx = Transaction {
-		version: TxVersion::TWO,
-		lock_time: LockTime::from_height(nodes[0].best_block_info().1).unwrap(),
-		input: vec![TxIn { ..Default::default() }],
-		output,
-	};
-	let height = nodes[0].best_block_info().1 + 1;
-	let block = create_dummy_block(nodes[0].best_block_hash(), height, vec![tx.clone()]);
-	for node in nodes {
-		do_connect_block_with_consistency_checks(node, block.clone(), false);
-	}
-	tx
+	provide_anchor_utxo_reserves(nodes, 1, Amount::ONE_BTC)
 }
 
 pub fn provide_anchor_utxo_reserves<'a, 'b, 'c>( | 
| Backported to 0.2 in #4185. | 
Otherwise, we could potentially hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4) if we aggregate enough HTLC-claims together.